Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Don't use temp folder name in output folder path #394

Merged
merged 1 commit into from
Feb 26, 2025

Conversation

alexdewar
Copy link
Collaborator

Description

We use the name of the folder the input files are in as a proxy for the model name, specifically when creating the output folder for the model run (e.g. you get something like muse2_results/my_model_name). Unfortunately when extracting the bundled example input files, we use a temporary folder, so you get something like muse2_results/.tmp4eU71Y instead. Fix by making a subfolder with a proper name in the temp folder.

Fixes #374.

Type of change

  • Bug fix (non-breaking change to fix an issue)
  • New feature (non-breaking change to add functionality)
  • Refactoring (non-breaking, non-functional change to improve maintainability)
  • Optimization (non-breaking change to speed up the code)
  • Breaking change (whatever its nature)
  • Documentation (improve or add documentation)

Key checklist

  • All tests pass: $ cargo test
  • The documentation builds and looks OK: $ cargo doc

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@alexdewar alexdewar requested a review from TinyMarsh February 17, 2025 10:24
Copy link

codecov bot commented Feb 17, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.31%. Comparing base (7733c8d) to head (ec46835).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/commands.rs 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   95.43%   95.31%   -0.13%     
==========================================
  Files          31       31              
  Lines        4007     4009       +2     
  Branches     4007     4009       +2     
==========================================
- Hits         3824     3821       -3     
- Misses         96      101       +5     
  Partials       87       87              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@TinyMarsh TinyMarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving because the code looks fine. But wanted to mention that I actually don't get a temp directory when running the example. Outputs just go straight to muse2_results/simple.

I wonder if this is a platform dependant thing? I'm on Windows.

@alexdewar
Copy link
Collaborator Author

You'll only see the bug if you use the example subcommand, e.g.:

cargo run example run simple

@alexdewar alexdewar force-pushed the fix-example-output-folder-name branch from 74fa465 to f286a49 Compare February 26, 2025 10:46
@alexdewar alexdewar force-pushed the fix-example-output-folder-name branch from f286a49 to ec46835 Compare February 26, 2025 10:54
@alexdewar alexdewar merged commit a3f60ab into main Feb 26, 2025
7 checks passed
@alexdewar alexdewar deleted the fix-example-output-folder-name branch February 26, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: temp folder name used for output folder if using example run command
2 participants